files: extract page header component#1608
files: extract page header component#1608ivaavimusic wants to merge 4 commits intorecoupable:testfrom
Conversation
|
@ivaavimusic is attempting to deploy a commit to the Recoupable Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis change adds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc06008f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!disableInternalScroll) { | ||
| return; |
There was a problem hiding this comment.
When disableInternalScroll flips from true to false (e.g., after sending the first message), adjustHeight exits early and never clears the inline textarea.style.height previously set during autosizing. This leaves the composer stuck at an expanded height even after the input is cleared, so the post-send chat input can remain oversized until remount. Add a reset path for the non-autosize branch (or whenever the flag changes) so height returns to the normal controlled size.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/Sandboxes/NoSandboxFiles.tsx (1)
24-30: Optional: remove duplicated width constraint to reduce style drift.
max-w-mdis applied on both wrapper and root dropzone; keeping it in one place makes future layout changes safer.♻️ Suggested simplification
- <div + <div {...getRootProps()} role="region" aria-label="File upload dropzone" className={cn( - "w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all", + "w-full rounded-lg p-8 border-2 border-dashed transition-all",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Sandboxes/NoSandboxFiles.tsx` around lines 24 - 30, The dropzone has a duplicated width constraint: the outer wrapper div and the inner element using {...getRootProps()} both include "max-w-md", which can cause style drift; remove "max-w-md" from the inner element's className (the element that spreads getRootProps()) and keep it on the outer wrapper div so the width constraint is defined in a single place, ensuring future layout changes are safer.components/Sidebar/RecentChats/ChatItem.tsx (1)
155-182: Align menu keyboard behavior withrole="menu"semantics.This menu currently handles Escape, but not arrow-key navigation/focus movement expected for menu patterns. Either implement full menu keyboard behavior or switch to semantics that match current interaction.
As per coding guidelines, "Support arrow key navigation in dropdown menu components" and "Implement proper focus management in dropdown menu components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Sidebar/RecentChats/ChatItem.tsx` around lines 155 - 182, The menu currently uses role="menu" but only handles Escape; update ChatItem.tsx to either implement proper menu keyboard/focus behavior or change semantics to match current interactions: either (A) implement arrow-key focus movement and roving focus inside the menu when isMenuOpen (manage focus on menuRef and menu items, handle ArrowDown/ArrowUp to move focus between the buttons, Home/End to jump, and Escape to close via onMenuToggle) and ensure the buttons (onRenameClick, onDeleteClick) are focusable and have role="menuitem" and appropriate aria-activedescendant/aria-controls as needed, or (B) remove role="menu" and use role="listbox" or no menu role to reflect basic click-only behavior; pick one approach and update ChatItem's menuRef, isMenuOpen conditional, onMenuToggle, onRenameClick and onDeleteClick accordingly to maintain correct focus cleanup when closing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/VercelChat/FileMentionsInput.tsx`:
- Around line 33-45: The adjustHeight callback exits early when
disableInternalScroll is false, leaving any previously set inline textarea
height intact; update adjustHeight (and callers if needed) so that when
disableInternalScroll is false it clears the inline height on the element
referenced by inputRef (e.g., set textarea.style.height = "" or "auto") before
returning, ensuring the composer resets to its default height; reference the
adjustHeight function, the disableInternalScroll flag, and
inputRef/textarea.style.height when making the change.
---
Nitpick comments:
In `@components/Sandboxes/NoSandboxFiles.tsx`:
- Around line 24-30: The dropzone has a duplicated width constraint: the outer
wrapper div and the inner element using {...getRootProps()} both include
"max-w-md", which can cause style drift; remove "max-w-md" from the inner
element's className (the element that spreads getRootProps()) and keep it on the
outer wrapper div so the width constraint is defined in a single place, ensuring
future layout changes are safer.
In `@components/Sidebar/RecentChats/ChatItem.tsx`:
- Around line 155-182: The menu currently uses role="menu" but only handles
Escape; update ChatItem.tsx to either implement proper menu keyboard/focus
behavior or change semantics to match current interactions: either (A) implement
arrow-key focus movement and roving focus inside the menu when isMenuOpen
(manage focus on menuRef and menu items, handle ArrowDown/ArrowUp to move focus
between the buttons, Home/End to jump, and Escape to close via onMenuToggle) and
ensure the buttons (onRenameClick, onDeleteClick) are focusable and have
role="menuitem" and appropriate aria-activedescendant/aria-controls as needed,
or (B) remove role="menu" and use role="listbox" or no menu role to reflect
basic click-only behavior; pick one approach and update ChatItem's menuRef,
isMenuOpen conditional, onMenuToggle, onRenameClick and onDeleteClick
accordingly to maintain correct focus cleanup when closing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 022d880d-fae1-416f-b459-4fcafde4208e
📒 Files selected for processing (9)
app/files/page.tsxcomponents/Sandboxes/NoSandboxFiles.tsxcomponents/Sidebar/NewChatButton.tsxcomponents/Sidebar/RecentChats/ChatItem.tsxcomponents/VercelChat/ChatInput.tsxcomponents/VercelChat/FileMentionsInput.tsxcomponents/VercelChat/mentionsStyles.tscomponents/ui/button.tsxcomponents/ui/dialog.tsx
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
app/files/page.tsx
Outdated
| <div className="px-6 md:px-12 py-8 space-y-6"> | ||
| <div className="max-w-3xl"> | ||
| <h1 className="text-left font-heading text-3xl font-bold"> | ||
| Files | ||
| </h1> | ||
| <p className="mt-2 text-lg text-muted-foreground font-light font-sans"> | ||
| Upload repository files for your agent sandbox, including code, docs, assets, and reference material. | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
Single Responsibility / Open Closed Principles
- actual: adding net new code to an existing component.
- required: new component file for the Header code.
There was a problem hiding this comment.
Makes sense. I’ll keep the header improvement but move it into a separate component file.
There was a problem hiding this comment.
For the state of missing files, we actually want these files to get setup via the Sandbox task.
- docs: https://developers.recoupable.com/api-reference/sandboxes/create
For now, I recommendrevertingthe changes to this file.
Once you finish this pull request, you can work on improving the experience formissing files.
There was a problem hiding this comment.
Understood. I was approaching this as a general empty-state improvement, but I see that the intended flow is different here. I’ll revert this file from the PR for now and keep missing-files UX as a separate follow-up.
| }} | ||
| className={cn( | ||
| "shrink-0 w-4 h-4 rounded border-2 transition-all duration-150 flex items-center justify-center", | ||
| "shrink-0 w-4 h-4 rounded border-2 transition-all duration-150 flex items-center justify-center cursor-pointer", |
There was a problem hiding this comment.
Single Responsibility Principle
- why are you modifying
chatItemin a pull request titledupload-guidance? - Each pull request should improve exactly one thing.
There was a problem hiding this comment.
Agreed. This is unrelated to upload guidance. I’ll remove it from this PR and keep it for a separate UI polish PR.
components/Sidebar/NewChatButton.tsx
Outdated
| type="button" | ||
| className={cn( | ||
| "inline-flex items-center h-10 rounded-lg whitespace-nowrap overflow-hidden transition-all duration-200 text-sm font-normal text-foreground hover:bg-muted", | ||
| "inline-flex items-center h-10 rounded-lg whitespace-nowrap overflow-hidden transition-all duration-200 text-sm font-normal text-foreground hover:bg-muted cursor-pointer", |
There was a problem hiding this comment.
Same SRP question here.
There was a problem hiding this comment.
Agreed. I’ll move this out of the PR so the scope stays focused.
components/ui/button.tsx
Outdated
|
|
||
| const buttonVariants = cva( | ||
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-xl text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0", | ||
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-xl text-sm font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 cursor-pointer", |
There was a problem hiding this comment.
Do not modify root shadcn components.
components/ui/dialog.tsx
Outdated
| > | ||
| {children} | ||
| <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground"> | ||
| <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground cursor-pointer"> |
There was a problem hiding this comment.
Do not modify root shadcn components.
components/VercelChat/ChatInput.tsx
Outdated
| onChange={setInput} | ||
| disabled={isDisabled || hasPendingUploads} | ||
| model={model} | ||
| disableInternalScroll={messages.length === 0} |
There was a problem hiding this comment.
SRP - why are you modifying chat components in a pr titled for file uploads?
There was a problem hiding this comment.
Fair point. This belongs in a separate PR, not this one. I’ll remove it from here.
There was a problem hiding this comment.
Could you please summarize what the purpose of these changes are and how they contribute to the title of the PR?
There was a problem hiding this comment.
You’re right to call this out. This was a misunderstood change on my side and it does not meaningfully contribute to the scope or title of this PR. I’ll remove it
There was a problem hiding this comment.
What is the purpose of these file changes?
There was a problem hiding this comment.
This file changed only in support of the same misunderstood change above, so it does not belong in this PR either. I’ll remove this as well.
ivaavimusic
changed the title
ivaavimusic
changed the title
| <div className="max-w-3xl"> | ||
| <h1 className="text-left font-heading text-3xl font-bold">Files</h1> | ||
| <p className="mt-2 text-lg text-muted-foreground font-light font-sans"> | ||
| Upload repository files for your agent sandbox, including code, docs, |
There was a problem hiding this comment.
| Upload repository files for your agent sandbox, including code, docs, | |
| Upload repository files for your agent, including images, songs, |
2 participants
Summary
This PR improves the Files page by adding a clear page header and short upload guidance.
Changes
FilesPageHeadercomponentWhy
The Files page previously had no title or context, which made the upload area feel ambiguous. This adds lightweight guidance without changing the underlying file flow.
Risk
Low. This is a small page-level UI change limited to header content and spacing.